Commit together with co-authors #1355

kuychaco · 2018-03-16T18:12:07Z

TODO:

update user store after initial loading (what if new commits are pulled with new authors?)
cache invalidation for getAuthors

In separate PRs:

use %x1F delimiter for getRecentCommits
hide behind feature flag
add new author (use prompt similar to CredentialDialog)
get username and avatar url from github graphql API
get rid of web committer (like so)
consider sorting based on most recent authors or # of commits rather than alphabetically
get rid of amend checkbox and implement undo and “amend” right-click instead
- as is, clicking the amend checkbox will not strip co-author trailers and you could end up with duplicate co-authors. so we'll want to make this change before shipping to beta

/cc @niik @simurai

Courtesy of GitHub Desktop :)

Co-Authored-By: Katrina Uychaco <[email protected]>

Todo: figure out a good number

Co-Authored-By: Tilde Ann Thurium <[email protected]>

…tion Co-Authored-By: Tilde Ann Thurium <[email protected]>

Co-Authored-By: Tilde Ann Thurium <[email protected]>

(to fix tests) Co-Authored-By: Tilde Ann Thurium <[email protected]>

Co-Authored-By: Tilde Ann Thurium <[email protected]>

simurai

How does the list get populated when searching for someone? Would it be possible to show username Real Name instead of Real Name [email protected]? Like GitHub Desktop.

Then it might also be possible to only show a person one time. Otherwise it's hard to decide which one to pick:

kuychaco · 2018-03-22T11:49:30Z

@simurai great question! This PR just pulled author information from Git itself - from the local repository. In another PR we'll grab info from the graphql API and allow people to search via username. We decided on this approach first because we want to cover the base case where the user has a repo but doesn't necessarily have a github remote.

smashwilson

Nice ✨ No blocking changes that I see. Merge whenever you're ready 😄

smashwilson · 2018-03-22T12:54:39Z

keymaps/git.cson

 '.github-Dialog input':
  'enter': 'core:confirm'
+
+'body .github-CommitView-coAuthorEditor':


Is body needed here for selector specificity reasons?

smashwilson · 2018-03-22T12:55:45Z

lib/controllers/commit-controller.js

+    mentionableUsers: PropTypes.arrayOf(PropTypes.shape({
+      email: PropTypes.string.isRequired,
+      name: PropTypes.string.isRequired,
+    })).isRequired,


👍 for PropTypes.shape(). I know we've been a bit sloppy with our PropTypes sometimes 😄 Although it'll be a moot point if we do go to TypeScript 🤔

smashwilson · 2018-03-22T12:58:49Z

lib/git-shell-out-strategy.js

      // There's probably a better way. I tried finding a regex to do it in one fell swoop but had no luck
      const coAuthors = body.split(LINE_ENDING_REGEX).reduce((emails, line) => {
-        const match = line.match(/\s*Co-authored-by: .*<(.*)>\s*/);
+        const match = line.match(/^co-authored-by: .*<(.*)>\s*/i);


Good call 👍

smashwilson · 2018-03-22T13:00:05Z

lib/git-shell-out-strategy.js

+        return [];
+      } else {
+        throw err;
+      }


smashwilson · 2018-03-22T13:00:46Z

lib/git-shell-out-strategy.js

+    for (const trailer of trailers) {
+      args.push('--trailer', `${trailer.token}=${trailer.value}`);
+    }
+    return this.exec(args, {stdin: commitMessage});


Huh! TIL 🌠

smashwilson · 2018-03-22T13:07:24Z

lib/views/commit-view.js

+      if (!fakeEvent.defaultPrevented) {
+        e.abortKeyBinding();
+      }
+    };


Huh, wacky 😄

smashwilson · 2018-03-22T13:09:14Z

lib/views/commit-view.js

+        'github:co-author:pagedown': this.proxyKeyCode(34),
+        'github:co-author:end': this.proxyKeyCode(35),
+        'github:co-author:home': this.proxyKeyCode(36),
+        'github:co-author:delete': this.proxyKeyCode(46),


Random sidenote: we could move these into a <Commands> view for extra React Points.

smashwilson · 2018-03-22T13:10:34Z

package.json

    "diff": "3.2.0",
    "dompurify": "^1.0.3",
-    "dugite": "1.49.0",
+    "dugite": "^1.60.0",


Ah good call.

smashwilson · 2018-03-22T13:10:53Z

package.json

    "FilePatchControllerStub": "createFilePatchControllerStub"
-  }
+  },
+  "false": {}


Hmm, what's this for?

smashwilson · 2018-03-22T13:11:32Z

test/git-strategies.test.js

+
+        await git.exec(['config', 'user.name', 'Com Mitter']);
+        await git.exec(['config', 'user.email', '[email protected]']);
+        await git.exec(['commit', '--allow-empty', '--author="A U Thor <[email protected]>"', '-m', 'Commit together!']);


smashwilson · 2018-03-22T13:16:07Z

Huh. What's up with the failing tests, though? They look unrelated... ?

See https://git-scm.com/docs/git-config#git-config-sshvariant for details.

kuychaco · 2018-03-26T20:59:26Z

For the sake of getting this feature out so that others can dog-food and provide input we're going to merge this as-is and implement all remaining to-dos in future PRs.

kuychaco and others added 30 commits March 14, 2018 18:31

WIP

84d8ee7

Implement Repository#getAuthors and tests

25a969f

Add getAuthors method to repository

9cb7861

Add basic UserStore

59ed707

Add Repository#formatCommitMessage and tests

5461d62

Courtesy of GitHub Desktop :)

WIP

90ab5f4

Add co-author toggle button

ca62c09

Add co-author editor

2c93f54

Add select list

1047c91

Add react-select dependency

f9a226d

Plumbing and wiring

5d51ca7

Tell Select where to find the label and value

4485f78

Store the select ref

811a51a

Look at all this sadness

5b8d218

A little bit of spacing

e01e6b9

Wire up the last key bindings

2d108f0

🔥 debug log

f0b3259

Only handle the event when the Select did

3d46981

Use an actual fake event

c5f05de

Co-Authored-By: Katrina Uychaco <[email protected]>

👕

086682e

Get more authors from repo (max: 5000)

5fd4dad

Todo: figure out a good number

Display author name if no login is available

c03c406

Display actual authors in select list

7518123

Case desensitize check for co-author-trailers in recent commits

f7dc9ea

Add selected co-authors as trailers in commit body

044b389

Rename formatCommitMessage => addTrailersToCommitMessage

33bc07a

Undo accidental package.json dependency change

23df4fc

👕

3187074

🔥 comment

6897161

🔥 whitespace

068c653

kuychaco and others added 9 commits March 21, 2018 17:36

📝 User Store

0629af0

Co-Authored-By: Tilde Ann Thurium <[email protected]>

Add authors cache key

20e54f4

Co-Authored-By: Tilde Ann Thurium <[email protected]>

Add title to toggle co-authors icon for accessibility

a9d4a9b

Co-Authored-By: Tilde Ann Thurium <[email protected]>

🔥 login until we add github user info

e6d23f4

Co-Authored-By: Tilde Ann Thurium <[email protected]>

Don't call UserStore#populate in tests since it happens upon construc…

46ef9cd

…tion Co-Authored-By: Tilde Ann Thurium <[email protected]>

Add User Store test for refetching data when HEAD changes

8a6ca82

Co-Authored-By: Tilde Ann Thurium <[email protected]>

Fix test

c93f83a

Co-Authored-By: Tilde Ann Thurium <[email protected]>

Trim message before committing and default coAuthors to empty array

5cdee60

(to fix tests) Co-Authored-By: Tilde Ann Thurium <[email protected]>

Add comment about caching

7cb00e9

Co-Authored-By: Tilde Ann Thurium <[email protected]>

simurai reviewed Mar 22, 2018

View reviewed changes

kuychaco and others added 4 commits March 21, 2018 23:04

👕

5329c86

Iterate less

b9161a6

Merge branch 'master' into ku-niik/co-authors

a467058

Describe shape of user in PropTypes

d51cbbe

kuychaco requested a review from smashwilson March 22, 2018 11:38

smashwilson approved these changes Mar 22, 2018

View reviewed changes

kuychaco and others added 5 commits March 22, 2018 13:21

🔥 package.json entry that eslint added

6bf127c

Remove body from keymap selector

f019b05

Merge branch 'master' into ku-niik/co-authors

c90bd8b

Set GIT_SSH_VARIANT to prevent git from doing a "dry run".

3285734

See https://git-scm.com/docs/git-config#git-config-sshvariant for details.

"git fetch" _does_ reject now. But only sometimes!

6be2b05

smashwilson mentioned this pull request Mar 24, 2018

Undo last commit and amend context menu option #1364

Merged

14 tasks

kuychaco merged commit b994822 into master Mar 26, 2018

kuychaco removed the work-in-progress label Apr 2, 2018

kuychaco mentioned this pull request Apr 3, 2018

Add dialog for new co-author #1374

Merged

33 tasks

smashwilson mentioned this pull request Apr 19, 2018

Co-author commits #1309

Closed

smashwilson deleted the ku-niik/co-authors branch September 21, 2018 00:46

Commit together with co-authors #1355

Commit together with co-authors #1355

Uh oh!

Conversation

kuychaco commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simurai left a comment

Choose a reason for hiding this comment

Uh oh!

kuychaco commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smashwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smashwilson commented Mar 22, 2018

Uh oh!

kuychaco commented Mar 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kuychaco commented Mar 16, 2018 •

edited

Loading

kuychaco commented Mar 22, 2018 •

edited

Loading